Closing merge file_descr when aborting #326
Conversation
Gives Without this PR and with this PR. It actually made me realise that there's another file descriptor that's not closed, |
test/unix/main.ml
Outdated
| @@ -714,6 +714,8 @@ module Close = struct | |||
| let* Context.{ rw; _ } = | |||
| Context.with_full_index ~throttle:`Block_writes ~size:100 () | |||
| in | |||
| let root = Index.root rw in | |||
There was a problem hiding this comment.
there is a root global variable already defined in this file, can you reuse it?
There was a problem hiding this comment.
Oh, yes, don't know how I missed it, thanks! And thanks for the review :-)
| @@ -188,3 +188,48 @@ let check_disjoint index htbl = | |||
| Alcotest.failf "Found value %a when checking for the absence of %a" | |||
| (Repr.pp Value.t) v' pp_binding (k, v)) | |||
| htbl | |||
|
|
|||
| let get_open_fd root = | |||
There was a problem hiding this comment.
there is a bit of code duplication with test_fd in the force_merge tests, can you maybe combine the two functions?
There was a problem hiding this comment.
Done. I replaced the /tmp directory used to look for the files with root since all the files are created there from what I can see
src/index.ml
Outdated
| @@ -1034,6 +1036,9 @@ struct | |||
|
|
|||
| let close = close' ~hook:(fun _ -> ()) | |||
|
|
|||
| let root it = | |||
There was a problem hiding this comment.
you don't need this function anymore, no?
test/unix/force_merge.ml
Outdated
| let lsof_command = "lsof -a -s -p " ^ pid ^ " > " ^ fd_file in | ||
| let lines = Common.get_open_fd root in | ||
| (* Not sure that looking in /tmp is correct since all the writes | ||
| * are done in root *) |
There was a problem hiding this comment.
good catch! let's remove this comment, it does not make sense now.
Fixes #325